-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
context capture screenshot: add optional focus top-level context step #440
Conversation
0ba219e
to
3e32d95
Compare
This PR was created as per @sadym-chromium's suggestion given our chromium mapper implementation. |
51145bc
to
455d03c
Compare
CDP's Page.captureScreenshot command[1] is blocked until the active context gets focus. [1]: https://chromedevtools.github.io/devtools-protocol/tot/Page/#method-captureScreenshot Follow-up-of: #183
455d03c
to
063de20
Compare
I'm not that happy about that change given that it might cause side-effects users aren't aware of. For example web pages in background tabs will in most cases get throttled and as such will behave differently as when they are in foreground. So I can see especially issues when the focus is not set back to the originally focused tab. I think that this PR needs a discussion. |
The alternative is to focus on whatever was focused previously, but we do not yet have the ability to determine that, correct? Maybe #171 can help with that. |
If some implementation specific steps have to be taken then such code could also revert the focus after the screenshot has been done. That's nothing the client has to be aware of. But again this is causing side-effects that you should be aware of. |
Arguably if Chrome can only take screenshots when the context has focus, it should return |
Will add an agenda item for this in the monthly meeting. |
Related: puppeteer/puppeteer#2254 |
Once #453 is landed, do you think it would be feasible to change the WPT tests for capture screenshot to first call If the answer is no: Then what if we added more tests instead? We could keep the current ones + add extra ones that perform the focus before taking screenshot. |
I think it would be fine to have screenshot tests that test foreground and background. Maybe the background tests only test the basic features then and all the available variants are done in a foreground tab. |
We will instead leverage #453 to explicitly focus. Otherwise the implementation will throw an error. WPT tests should be updated accordingly once the command is added to the spec (as whimboo@ seems to agree above). |
Note that while our tests can easily call the extra activate command and test both scenarios, Webdriver BiDi users will wonder why they have to explicitly activate the tab before taking a screenshot. As such it would be good if Chrome could fix that issue and allow capturing screenshots in background tabs. |
For completeness: https://bugs.chromium.org/p/chromium/issues/detail?id=32667 to support background screenshots |
This issue was closed because of: (i) web-platform-tests/wpt#40965 and (ii) WG consensus that this is probably not a good idea. From @jgraham via IRC:
|
CDP's Page.captureScreenshot command1 is blocked until the active context gets focus.
Follow-up-of: #183
Spec: https://w3c.github.io/webdriver-bidi/#command-browsingContext-captureScreenshot
Preview | Diff